fix: rate limit signup, recover, and verify endpoints#450
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change updates route registration in Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
…miting-on-signup-recovery-or-verify
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/api.go (1)
132-155: ⚡ Quick winAdd regression tests for the new limiter wiring and endpoint-bucket isolation.
This security-sensitive change adds three new rate-limited routes but the provided coverage evidence only shows
/tokenbehavior (api/token_test.go:136-165). Please add tests for/signup,/recover, and/verifythat assert 429 at the configured thresholds and confirm one endpoint’s bucket does not throttle the others for the same client key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/api.go` around lines 132 - 155, Add regression tests covering the three new rate-limited routes (handlers api.Signup, api.Recover, api.Verify) and an isolation test proving one endpoint’s bucket does not throttle others; for each route create tests that send requests with the same client key/header and assert HTTP 429 once the configured limit is exceeded (Signup: >10 requests/min, Recover: >5 requests/min, Verify: >30 requests/min; Token already covered), and add a test that floods one endpoint (e.g., signup) to 429 then immediately calls another endpoint (e.g., recover or token) with the same client key and asserts it is not throttled; ensure tests use the same request helpers/middleware as the server (limitHandler, requireEmailProvider) and reset/clear any in-memory limiter state between subtests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@api/api.go`:
- Around line 132-155: Add regression tests covering the three new rate-limited
routes (handlers api.Signup, api.Recover, api.Verify) and an isolation test
proving one endpoint’s bucket does not throttle others; for each route create
tests that send requests with the same client key/header and assert HTTP 429
once the configured limit is exceeded (Signup: >10 requests/min, Recover: >5
requests/min, Verify: >30 requests/min; Token already covered), and add a test
that floods one endpoint (e.g., signup) to 429 then immediately calls another
endpoint (e.g., recover or token) with the same client key and asserts it is not
throttled; ensure tests use the same request helpers/middleware as the server
(limitHandler, requireEmailProvider) and reset/clear any in-memory limiter state
between subtests.
…miting-on-signup-recovery-or-verify
…miting-on-signup-recovery-or-verify
VaibhavAcharya
deleted the
vaibhavacharya/ex-1858-identity-no-rate-limiting-on-signup-recovery-or-verify
branch
2 participants
- Summary
Only
/tokenwas rate-limited, leaving/signupopen to email flooding,/recoveropen to account enumeration and mass recovery emails, and/verifyopen to confirmation-token brute force. This adds independenttollboothlimiters per endpoint: 10/min on/signup, 5/min on/recover, 30/min on/verify, each with its own bucket so a flood on one endpoint doesn't lock out the others.- Test plan
- Description for the changelog
Rate limit
/signup,/recover, and/verifyper client.- A picture of a cute animal (not mandatory but encouraged)